Simplify ACL request refresh detection (improves #4409) #4483
Conversation
MohabCodeX
force-pushed
the
fix/improve-aclrequest-detection
branch
from
9f17f56 to
07cb21d
Compare
|
related #4484 |
|
|
||
| std::string CalculateACLRequestFingerprint(); | ||
| bool HasACLRequestsChanged(); | ||
| protected: |
There was a problem hiding this comment.
For accessing members, but i made a better solution now 👍🏻
|
Is this really a fix? Or this became an oversimplification refactor over a bug? |
|
Yes it fixes #4484 I have tested it. |
|
I've written my concerns about this and the prior PR: |
This PR basically does revert that previous PR and replaces it with a simpler fix for the original bug. How it works now is that if a resource has ACL requests, it doesn't have to be started to output the requests to the server console, if you changed them and did refresh, the changes wouldn't be registered, but now when doing refresh, the changes are registered to these non started resources. Currently the only way to get the ACL requests to get registered is to rename the resource and execute refresh. Now you don't have to rename a resource. |
|
refreshall triggers the acl reload check |
|
That's true, but you may not want to restart all changed resources, it can be very slow and freeze the server for 1 minute, or you just might not want some changes to be applied. |
|
We can agree that you brought into the table that the culprit of the preference, is a performance issue.
This is to be addressed sometime using CryptoPP implementation instead of handrolled and see if the performance is improved (Which I assume will improve at least 40%) |
This PR replaces the complex ACL fingerprinting from PR #4409 with a much simpler and more efficient approach.
Thanks for @ArranTuna for mentioning a proper solution.